Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SAK-50454 polls improve ui responsiveness #12856

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Sharadhi98
Copy link
Contributor

I have resolved the responsiveness issue of the polls tool by using table-responsive. I observed that the grid system applied to the portal-main-container was interfering with the styling of table-responsive, so I replaced it with flex and added two classes: d-flex and flex-column.
I'd love some feedback on changing the portal-main-container from grid to flex.
Thanks!


<p class="act">
<p class="act" style="flex-wrap:nowrap;">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer not using inline style

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I updated the code with a bootstrap class instead.

@ern ern changed the title SAK-50454 Responsiveness of Polls tool SAK-50454 polls improve ui responsiveness Sep 10, 2024
@@ -108,9 +109,9 @@ <h1 rsf:id="poll-list-title">Polls</h1>
</td>
</tr>
</tbody>
</table>
</table></div>
Copy link
Member

@kunaljaykam kunaljaykam Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Just a quick suggestion: placing </table> and </div> on separate lines improves readability and maintainability. Could you adjust the formatting?

Removed redundant javascript script types

Indented all the markup properly

We need to remove those vula references at some point :)
Copy link
Contributor

@adrianfish adrianfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added another commit. Don't be afraid to remove redundant code, like the javascript script type on script tags. If I'm working in a file and I see old stuff that might not actually be related to the thing I'm fixing, I will still clean it up. That way our codebase steadily improves. Don't go crazy and start looking at the entire polls tool, but changes to the area where you're working are fine.

Copy link
Member

@kunaljaykam kunaljaykam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! However, I noticed there's no i18n support, @adrianfish Is it okay?

@Sharadhi98 , please test one last time with the latest changes, and once everything looks good, we can merge. Thanks!

</div>
</body>
</html>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<!DOCTYPE html>

Comment on lines +8 to +9
<script>includeLatestJQuery('polls');</script>
<script>includeWebjarLibrary('jquery.tablesorter');</script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<script>includeLatestJQuery('polls');</script>
<script>includeWebjarLibrary('jquery.tablesorter');</script>
<script>
includeLatestJQuery('polls');
includeWebjarLibrary('jquery.tablesorter');
</script>

@Sharadhi98
Copy link
Contributor Author

Looks good overall! However, I noticed there's no i18n support, @adrianfish Is it okay?

@Sharadhi98 , please test one last time with the latest changes, and once everything looks good, we can merge. Thanks!

For some reason, I'm unable to test the Polls tool. Even though there's are no compilation errors, I get an error when I load the tool on my local. Here's a screenshot.
Screen Shot 2024-09-23 at 6 15 56 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants